Add workspace tabs as a display option (new default) - #3394
Conversation
Introduces a horizontal tab bar at the top of the window as an alternative workspace switcher, selectable from the View menu alongside the existing vertical sidebar. Tabs are the new default on first launch and after upgrade; user selection persists. - Add TabBar component tree (WorkspaceTab, WindowControls, MeatballMenuButton, useTabBarLayout) with Chrome-style progressive tab condensing and a reserved drag region - Add navigationLayout reducer/persistence to track bar-vs-tabs choice - Wire View menu toggle (macOS native menu, Windows meatball popup) mutually exclusive between Workspace Bar and Workspace Tabs - Move useKeyboardShortcuts/useSorting out of SideBar into shared components/hooks so both displays reuse the same logic - Extend rootWindow/menuBar/serverView for custom window chrome on Windows and per-server keyboard shortcut accelerators
- Add Cmd/Ctrl+Tab and Cmd/Ctrl+Shift+Tab accelerators to cycle through workspaces, matching preserved-shortcut requirement (AC8) - Give each workspace tab/pane a stable id and wire role=tabpanel / aria-labelledby on the server content area for full tablist/tab/ tabpanel semantics (AC23) - Open the Windows meatball menu on a solo Alt key press, so it acts as the sole application-menu entry point like a native menu bar (AC18/AC23); ignores Alt used as a modifier for other shortcuts
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR adds a persisted ChangesNavigation Layout Feature
Estimated code review effort: 4 (Complex) | ~75 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
src/ui/components/SettingsView/features/MenuBar.tsx (1)
43-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStale wording in disabled hint after switching gate from
isSideBarEnabledtonavigationLayout.The disabled hint text is
"Cannot disable menu bar when the workspace bar is disabled...", but the condition that now triggers it isnavigationLayout !== 'sidebar'(i.e., the user is in tabs layout), not literally "workspace bar disabled" as a standalone toggle. Consider updating the copy to reflect that the menu bar is required while using Workspace Tabs, to avoid confusing users on Linux.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/SettingsView/features/MenuBar.tsx` around lines 43 - 47, The disabled hint in MenuBar is now triggered by navigationLayout !== 'sidebar', so the existing wording about the workspace bar being disabled is stale. Update the copy used in the description branch for MenuBar to match the actual gate and explain that the menu bar is required while using Workspace Tabs, keeping the condition and translation key usage aligned with navigationLayout and isMenuBarEnabled.src/i18n/en.i18n.json (1)
303-314: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOld
settings.options.sidebarblock likely orphaned.The PR replaces the SideBar settings feature with
NavigationLayout, but the originalsidebarblock (title/description/disabledHint) undersettings.optionsis left in place alongside the newnavigationblock. If theSideBarsettings component was removed, as implied by the PR summary, these three keys are now dead translation entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/i18n/en.i18n.json` around lines 303 - 314, The old settings.options.sidebar translation block appears to be orphaned now that NavigationLayout replaces the SideBar settings feature. Remove the unused sidebar keys from en.i18n.json and keep only the active navigation-related entries so the translations match the current Settings components and unique symbols like navigation and settings.options stay aligned with the UI.src/ui/main/menuBar.ts (2)
883-892: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createFileMenucan render an empty submenu.When
isAddNewServersEnabledis false,submenuresolves to[], producing a top-level "File" menu item with no entries in the Windows popup. Consider hiding the whole menu section or adding a fallback item in that case.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/main/menuBar.ts` around lines 883 - 892, The createFileMenu selector can produce an empty submenu when isAddNewServersEnabled is false, leaving a blank File menu item. Update createFileMenu in menuBar.ts to either return no menu entry at all in that case or provide a fallback submenu item, using the existing createAddNewServerMenuItem and on helpers to keep the File menu non-empty.
48-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winApply the new factories to remaining duplicate inline blocks.
createAboutMenuItem()andcreateAddNewServerMenuItem()were extracted here, but two pre-existing inline duplicates of this exact logic still exist elsewhere in the file: the "about" item increateHelpMenu's non-darwin branch (~line 837-848) and the "addNewServer" item increateWindowMenu's darwin branch (~line 501-514). Since this diff introduces the factory pattern specifically to avoid this duplication, consider reusing it in those two spots as well for consistency.♻️ Example fix for the two remaining duplicates
...on(process.platform === 'darwin' && isAddNewServersEnabled, () => [ - { - id: 'addNewServer', - label: t('menus.addNewServer'), - accelerator: 'CommandOrControl+N', - click: async () => { - const browserWindow = await getRootWindow(); - - if (!browserWindow.isVisible()) { - browserWindow.showInactive(); - } - browserWindow.focus(); - dispatch({ type: MENU_BAR_ADD_NEW_SERVER_CLICKED }); - }, - }, + createAddNewServerMenuItem(), { type: 'separator' }, ]),...on(process.platform !== 'darwin', () => [ - { - id: 'about', - label: t('menus.about', { appName: app.name }), - click: async () => { - const browserWindow = await getRootWindow(); - - if (!browserWindow.isVisible()) { - browserWindow.showInactive(); - } - browserWindow.focus(); - dispatch({ type: MENU_BAR_ABOUT_CLICKED }); - }, - }, + createAboutMenuItem(), ]),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/main/menuBar.ts` around lines 48 - 93, The new factory helpers in createAboutMenuItem and createAddNewServerMenuItem are only partially applied, leaving duplicate inline menu item logic in createHelpMenu’s non-darwin branch and createWindowMenu’s darwin branch. Replace those remaining inline “about” and “addNewServer” blocks with the existing factory helpers so the menu definitions stay consistent and centralized.src/app/main/data.spec.ts (1)
136-147: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't isolate which guard condition is exercised.
Setting both
isMenuBarEnabled: trueandnavigationLayout: 'sidebar'together means this test can't tell you which condition in thedata.tsfallback (!values.isMenuBarEnabledornavigationLayout !== 'sidebar') is actually preventing the mutation. Consider adding a case withisMenuBarEnabled: true, navigationLayout: 'tabs'to fully cover the guard combinations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/main/data.spec.ts` around lines 136 - 147, The test in data.spec.ts does not clearly isolate the guard in the data.ts fallback because it sets both isMenuBarEnabled and navigationLayout to values that satisfy the same branch. Update the scenario around the mockSelect call for the “should not modify settings when menubar is already enabled” case so it specifically exercises the !values.isMenuBarEnabled guard with a contrasting navigationLayout value, and add a separate case that uses isMenuBarEnabled: true with navigationLayout: 'tabs' to cover the other guard combination in the data.ts logic.src/ui/main/rootWindow.ts (1)
366-378: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate window-state dispatch logic; confirm dual immediate+debounced registration is intentional.
dispatchWindowStateImmediatelyduplicatesfetchAndDispatchWindowState's body (fetch state, dispatchROOT_WINDOW_STATE_CHANGED, dev-only warn on error), differing only in the dispatch target and debounce. Both are now wired to the samemaximize/unmaximizeevents, so every toggle triggers two state fetches/dispatches (one viadispatch, one viadispatchLocal~1s later).♻️ Proposed consolidation
+const dispatchWindowState = async ( + dispatchFn: typeof dispatch | typeof dispatchLocal +): Promise<void> => { + try { + const state = await fetchRootWindowState(); + dispatchFn({ type: ROOT_WINDOW_STATE_CHANGED, payload: state }); + } catch (error) { + if (process.env.NODE_ENV === 'development') { + console.warn('Failed to fetch window state:', error); + } + } +}; + - const fetchAndDispatchWindowState = debounce(async (): Promise<void> => { - try { - const state = await fetchRootWindowState(); - dispatchLocal({ - type: ROOT_WINDOW_STATE_CHANGED, - payload: state, - }); - } catch (error) { - if (process.env.NODE_ENV === 'development') { - console.warn('Failed to fetch window state:', error); - } - } - }, 1000); + const fetchAndDispatchWindowState = debounce( + () => dispatchWindowState(dispatchLocal), + 1000 + );and:
- const dispatchWindowStateImmediately = async (): Promise<void> => { - try { - const state = await fetchRootWindowState(); - dispatch({ - type: ROOT_WINDOW_STATE_CHANGED, - payload: state, - }); - } catch (error) { - if (process.env.NODE_ENV === 'development') { - console.warn('Failed to fetch window state:', error); - } - } - }; + const dispatchWindowStateImmediately = () => + dispatchWindowState(dispatch);Please confirm the dual immediate/debounced registration for
maximize/unmaximizeis intentional (e.g., immediate UI feedback for the newWindowControlsicon vs. debounced sync elsewhere) rather than leftover duplication.Also applies to: 394-412
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/main/rootWindow.ts` around lines 366 - 378, The maximize/unmaximize handlers in rootWindow.ts are dispatching window state twice because dispatchWindowStateImmediately and fetchAndDispatchWindowState both fetch and emit ROOT_WINDOW_STATE_CHANGED with nearly identical logic. Confirm whether the immediate dispatch is intentionally needed for instant UI feedback; if not, consolidate the shared fetch/dispatch/error handling into one path and register only the required immediate or debounced listener so the same event does not trigger duplicate state updates.src/ui/components/TabBar/useTabBarLayout.ts (1)
44-67: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMemoize
tabListRefto avoid re-observing on every render.
tabListRefis a new function on every call touseTabBarLayout. Because it's used as a callback ref, React detaches (null) and reattaches it on every re-render of the consumer, causingunobserve/observechurn each time — andResizeObserver.observe()re-fires its callback immediately, scheduling an extra RAF + state update per render.♻️ Proposed fix
-import { useEffect, useRef, useState } from 'react'; +import { useCallback, useEffect, useRef, useState } from 'react'; ... - const tabListRef = (node: HTMLElement | null): void => { + const tabListRef = useCallback((node: HTMLElement | null): void => { if (observerRef.current && elementRef.current) { observerRef.current.unobserve(elementRef.current); } elementRef.current = node; if (node && observerRef.current) { observerRef.current.observe(node); } - }; + }, []);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/ui/components/TabBar/useTabBarLayout.ts` around lines 44 - 67, Memoize the tabListRef callback in useTabBarLayout so it stays stable across renders and does not trigger unnecessary detach/reattach behavior in consumers. Right now the callback ref recreates on every render, causing observerRef/unobserve-observe churn and extra ResizeObserver updates; wrap tabListRef with a stable callback mechanism and keep its logic for elementRef, observerRef, and availableWidth unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/i18n/en.i18n.json`:
- Around line 523-536: The tabBar unread message localization uses the old
i18next plural key pattern, which won’t resolve with the current i18next setup.
Update the `tabBar` entries in `en.i18n.json` to CLDR-style plural keys for the
`unreadMessage` string pair, or alternatively ensure every i18next
initialization path opts into v3 compatibility; use the `tabBar` and
`unreadMessage` keys to locate and align the pluralization format consistently.
In `@src/ui/components/ServersView/ServerPane.tsx`:
- Around line 210-218: The tab panel in ServerPane still references
getServerTabId(serverUrl), but overflowed servers no longer have a mounted
role="tab" element because TabBar only renders visibleServers. Update the tab
labeling approach so every server keeps an id-bearing tab in the DOM, or adjust
ServerPane’s aria-labelledby/tabpanel wiring to avoid pointing at missing tab
elements when a server is condensed.
In `@src/ui/components/SettingsView/features/NavigationLayout.tsx`:
- Around line 58-62: The group title label in NavigationLayout is incorrectly
tied to a specific radio via htmlFor={workspaceTabsId}, which makes the section
heading act like a control for “Workspace Tabs” instead of just labeling the
group. Update the title FieldLabel so it is not bound to either radio option,
and keep the individual option labels in the same component responsible for the
radio choices (the FieldLabel usage around the workspace tabs and workspace
sidebar options).
In `@src/ui/components/utils/getServerDomId.ts`:
- Around line 1-8: The DOM id generation in sanitize(), getServerTabId(), and
getServerPanelId() can collide for distinct URLs because punctuation is stripped
too aggressively. Update the id generation to preserve a unique suffix derived
from the full URL so urls that sanitize to the same base still produce different
workspace-tab-* and workspace-panel-* ids. Keep the existing helpers, but make
the suffix stable and consistent between getServerTabId and getServerPanelId so
aria-controls and aria-labelledby remain paired correctly.
In `@src/ui/reducers/navigationLayout.spec.ts`:
- Around line 68-84: The test case named “should handle undefined payload
gracefully” in navigationLayout.spec is misleading because it still passes an
empty object, so it does not cover the undefined-payload path. Update that test
to actually send an undefined payload for APP_SETTINGS_LOADED, and verify the
navigationLayout reducer still returns the default state; if the reducer’s
destructuring in navigationLayout cannot handle undefined, add a guard in the
reducer first so it safely falls back before destructuring.
---
Nitpick comments:
In `@src/app/main/data.spec.ts`:
- Around line 136-147: The test in data.spec.ts does not clearly isolate the
guard in the data.ts fallback because it sets both isMenuBarEnabled and
navigationLayout to values that satisfy the same branch. Update the scenario
around the mockSelect call for the “should not modify settings when menubar is
already enabled” case so it specifically exercises the !values.isMenuBarEnabled
guard with a contrasting navigationLayout value, and add a separate case that
uses isMenuBarEnabled: true with navigationLayout: 'tabs' to cover the other
guard combination in the data.ts logic.
In `@src/i18n/en.i18n.json`:
- Around line 303-314: The old settings.options.sidebar translation block
appears to be orphaned now that NavigationLayout replaces the SideBar settings
feature. Remove the unused sidebar keys from en.i18n.json and keep only the
active navigation-related entries so the translations match the current Settings
components and unique symbols like navigation and settings.options stay aligned
with the UI.
In `@src/ui/components/SettingsView/features/MenuBar.tsx`:
- Around line 43-47: The disabled hint in MenuBar is now triggered by
navigationLayout !== 'sidebar', so the existing wording about the workspace bar
being disabled is stale. Update the copy used in the description branch for
MenuBar to match the actual gate and explain that the menu bar is required while
using Workspace Tabs, keeping the condition and translation key usage aligned
with navigationLayout and isMenuBarEnabled.
In `@src/ui/components/TabBar/useTabBarLayout.ts`:
- Around line 44-67: Memoize the tabListRef callback in useTabBarLayout so it
stays stable across renders and does not trigger unnecessary detach/reattach
behavior in consumers. Right now the callback ref recreates on every render,
causing observerRef/unobserve-observe churn and extra ResizeObserver updates;
wrap tabListRef with a stable callback mechanism and keep its logic for
elementRef, observerRef, and availableWidth unchanged.
In `@src/ui/main/menuBar.ts`:
- Around line 883-892: The createFileMenu selector can produce an empty submenu
when isAddNewServersEnabled is false, leaving a blank File menu item. Update
createFileMenu in menuBar.ts to either return no menu entry at all in that case
or provide a fallback submenu item, using the existing
createAddNewServerMenuItem and on helpers to keep the File menu non-empty.
- Around line 48-93: The new factory helpers in createAboutMenuItem and
createAddNewServerMenuItem are only partially applied, leaving duplicate inline
menu item logic in createHelpMenu’s non-darwin branch and createWindowMenu’s
darwin branch. Replace those remaining inline “about” and “addNewServer” blocks
with the existing factory helpers so the menu definitions stay consistent and
centralized.
In `@src/ui/main/rootWindow.ts`:
- Around line 366-378: The maximize/unmaximize handlers in rootWindow.ts are
dispatching window state twice because dispatchWindowStateImmediately and
fetchAndDispatchWindowState both fetch and emit ROOT_WINDOW_STATE_CHANGED with
nearly identical logic. Confirm whether the immediate dispatch is intentionally
needed for instant UI feedback; if not, consolidate the shared
fetch/dispatch/error handling into one path and register only the required
immediate or debounced listener so the same event does not trigger duplicate
state updates.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66e75151-38c1-43fd-bee2-1338b9d0588c
📒 Files selected for processing (50)
src/app/PersistableValues.tssrc/app/__tests__/PersistableValues.spec.tssrc/app/main/data.spec.tssrc/app/main/data.tssrc/app/selectors.tssrc/i18n/en.i18n.jsonsrc/store/rootReducer.tssrc/ui/actions.tssrc/ui/common.tssrc/ui/components/DownloadsManagerView/index.spec.tsxsrc/ui/components/DownloadsManagerView/index.tsxsrc/ui/components/ServersView/ServerPane.tsxsrc/ui/components/ServersView/index.tsxsrc/ui/components/SettingsView/GeneralTab.tsxsrc/ui/components/SettingsView/SettingsView.tsxsrc/ui/components/SettingsView/features/MenuBar.tsxsrc/ui/components/SettingsView/features/NavigationLayout.spec.tsxsrc/ui/components/SettingsView/features/NavigationLayout.tsxsrc/ui/components/SettingsView/features/SideBar.tsxsrc/ui/components/Shell/index.spec.tsxsrc/ui/components/Shell/index.tsxsrc/ui/components/SideBar/ServerButton.tsxsrc/ui/components/SideBar/index.spec.tsxsrc/ui/components/SideBar/index.tsxsrc/ui/components/TabBar/CloseGlyph.tsxsrc/ui/components/TabBar/MaximizeGlyph.tsxsrc/ui/components/TabBar/MeatballMenuButton.spec.tsxsrc/ui/components/TabBar/MeatballMenuButton.tsxsrc/ui/components/TabBar/MinimizeGlyph.tsxsrc/ui/components/TabBar/RestoreGlyph.tsxsrc/ui/components/TabBar/WindowControls.spec.tsxsrc/ui/components/TabBar/WindowControls.tsxsrc/ui/components/TabBar/WindowsTitleBar.tsxsrc/ui/components/TabBar/WorkspaceTab.tsxsrc/ui/components/TabBar/index.spec.tsxsrc/ui/components/TabBar/index.tsxsrc/ui/components/TabBar/styles.tsxsrc/ui/components/TabBar/useTabBarLayout.spec.tssrc/ui/components/TabBar/useTabBarLayout.tssrc/ui/components/hooks/useKeyboardShortcuts.tsxsrc/ui/components/hooks/useSorting.tsxsrc/ui/components/utils/getServerDomId.tssrc/ui/components/utils/getServerInitials.tssrc/ui/main/menuBar.tssrc/ui/main/rootWindow.tssrc/ui/main/serverView/index.tssrc/ui/preload/sidebar.tssrc/ui/reducers/currentView.tssrc/ui/reducers/navigationLayout.spec.tssrc/ui/reducers/navigationLayout.ts
💤 Files with no reviewable changes (4)
- src/ui/reducers/currentView.ts
- src/ui/components/SettingsView/features/SideBar.tsx
- src/ui/components/DownloadsManagerView/index.tsx
- src/ui/components/DownloadsManagerView/index.spec.tsx
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: build (windows-latest, windows)
- GitHub Check: check (macos-latest)
- GitHub Check: check (windows-latest)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{tsx,ts}: MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed
Import UI components from@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens
Use React functional components with hooks
Use PascalCase for component file names
Files:
src/ui/components/SettingsView/features/NavigationLayout.spec.tsxsrc/ui/components/TabBar/WindowControls.tsxsrc/ui/components/utils/getServerInitials.tssrc/ui/components/TabBar/useTabBarLayout.spec.tssrc/ui/components/utils/getServerDomId.tssrc/ui/common.tssrc/ui/components/TabBar/CloseGlyph.tsxsrc/store/rootReducer.tssrc/ui/components/TabBar/RestoreGlyph.tsxsrc/ui/components/ServersView/index.tsxsrc/app/__tests__/PersistableValues.spec.tssrc/ui/components/TabBar/MaximizeGlyph.tsxsrc/ui/components/TabBar/MinimizeGlyph.tsxsrc/ui/components/SettingsView/features/MenuBar.tsxsrc/ui/components/TabBar/WindowControls.spec.tsxsrc/app/main/data.tssrc/ui/components/TabBar/WindowsTitleBar.tsxsrc/ui/components/TabBar/MeatballMenuButton.spec.tsxsrc/ui/main/serverView/index.tssrc/ui/preload/sidebar.tssrc/ui/components/SideBar/index.tsxsrc/ui/reducers/navigationLayout.spec.tssrc/ui/reducers/navigationLayout.tssrc/ui/components/SettingsView/features/NavigationLayout.tsxsrc/ui/components/SettingsView/GeneralTab.tsxsrc/ui/components/TabBar/MeatballMenuButton.tsxsrc/ui/components/TabBar/WorkspaceTab.tsxsrc/ui/components/TabBar/index.tsxsrc/ui/components/Shell/index.tsxsrc/ui/components/SideBar/ServerButton.tsxsrc/app/PersistableValues.tssrc/ui/components/ServersView/ServerPane.tsxsrc/ui/components/SideBar/index.spec.tsxsrc/ui/components/TabBar/useTabBarLayout.tssrc/ui/components/TabBar/index.spec.tsxsrc/ui/components/Shell/index.spec.tsxsrc/app/selectors.tssrc/ui/components/TabBar/styles.tsxsrc/ui/components/SettingsView/SettingsView.tsxsrc/ui/main/rootWindow.tssrc/ui/actions.tssrc/app/main/data.spec.tssrc/ui/main/menuBar.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Redux actions must follow FSA (Flux Standard Action) pattern
Avoid unnecessary comments — write self-documenting code through clear naming
Always verify libraries by checking official docs and.d.tsfiles innode_modules/. Never assume props, tokens, or APIs work without verification
Avoid subjective descriptors ('smart', 'excellent', 'dumb') in documentation and comments
Use measurable descriptions in code documentation: 'reduced memory usage', 'improved by X%' instead of subjective claims
NEVER invent metrics — don't include estimated time spent or speculated user counts. Only include numbers from actual logs, error messages, or documented sources
Files:
src/ui/components/SettingsView/features/NavigationLayout.spec.tsxsrc/ui/components/TabBar/WindowControls.tsxsrc/ui/components/utils/getServerInitials.tssrc/ui/components/TabBar/useTabBarLayout.spec.tssrc/ui/components/utils/getServerDomId.tssrc/ui/common.tssrc/ui/components/TabBar/CloseGlyph.tsxsrc/store/rootReducer.tssrc/ui/components/TabBar/RestoreGlyph.tsxsrc/ui/components/ServersView/index.tsxsrc/app/__tests__/PersistableValues.spec.tssrc/ui/components/TabBar/MaximizeGlyph.tsxsrc/ui/components/TabBar/MinimizeGlyph.tsxsrc/ui/components/SettingsView/features/MenuBar.tsxsrc/ui/components/TabBar/WindowControls.spec.tsxsrc/app/main/data.tssrc/ui/components/TabBar/WindowsTitleBar.tsxsrc/ui/components/TabBar/MeatballMenuButton.spec.tsxsrc/ui/main/serverView/index.tssrc/ui/preload/sidebar.tssrc/ui/components/SideBar/index.tsxsrc/ui/reducers/navigationLayout.spec.tssrc/ui/reducers/navigationLayout.tssrc/ui/components/SettingsView/features/NavigationLayout.tsxsrc/ui/components/SettingsView/GeneralTab.tsxsrc/ui/components/TabBar/MeatballMenuButton.tsxsrc/ui/components/TabBar/WorkspaceTab.tsxsrc/ui/components/TabBar/index.tsxsrc/ui/components/Shell/index.tsxsrc/ui/components/SideBar/ServerButton.tsxsrc/app/PersistableValues.tssrc/ui/components/ServersView/ServerPane.tsxsrc/ui/components/SideBar/index.spec.tsxsrc/ui/components/TabBar/useTabBarLayout.tssrc/ui/components/TabBar/index.spec.tsxsrc/ui/components/Shell/index.spec.tsxsrc/app/selectors.tssrc/ui/components/TabBar/styles.tsxsrc/ui/components/SettingsView/SettingsView.tsxsrc/ui/main/rootWindow.tssrc/ui/actions.tssrc/app/main/data.spec.tssrc/ui/main/menuBar.ts
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Use TypeScript for all new code unless explicitly told otherwise
Use optional chaining with fallbacks for platform-specific APIs instead of mocking when possible. Example:const uid = process.getuid?.() ?? 1000;
Files:
src/ui/components/utils/getServerInitials.tssrc/ui/components/TabBar/useTabBarLayout.spec.tssrc/ui/components/utils/getServerDomId.tssrc/ui/common.tssrc/store/rootReducer.tssrc/app/__tests__/PersistableValues.spec.tssrc/app/main/data.tssrc/ui/main/serverView/index.tssrc/ui/preload/sidebar.tssrc/ui/reducers/navigationLayout.spec.tssrc/ui/reducers/navigationLayout.tssrc/app/PersistableValues.tssrc/ui/components/TabBar/useTabBarLayout.tssrc/app/selectors.tssrc/ui/main/rootWindow.tssrc/ui/actions.tssrc/app/main/data.spec.tssrc/ui/main/menuBar.ts
**/*.spec.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Use
*.spec.tsfile naming for Renderer process tests
Files:
src/ui/components/TabBar/useTabBarLayout.spec.tssrc/app/__tests__/PersistableValues.spec.tssrc/ui/reducers/navigationLayout.spec.tssrc/app/main/data.spec.ts
**/*.{spec.ts,main.spec.ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Only mock platform-specific APIs when defensive coding isn't possible. Linux-only APIs requiring mocks:
process.getuid(),process.getgid(),process.geteuid(),process.getegid()
Files:
src/ui/components/TabBar/useTabBarLayout.spec.tssrc/app/__tests__/PersistableValues.spec.tssrc/ui/reducers/navigationLayout.spec.tssrc/app/main/data.spec.ts
🧠 Learnings (5)
📚 Learning: 2026-06-26T18:14:11.817Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/E2ePdfPreviewSizeLimit.tsx:47-55
Timestamp: 2026-06-26T18:14:11.817Z
Learning: In the Rocket.Chat Electron App SettingsView features under `src/ui/components/SettingsView/features/`, treat full-width selects/inputs (including full-width numeric inputs) as intentional for the stacked label/description layout. Per the UXDQA Figma spec (and macOS 1:1 verification), reviews should not flag these as layout regressions as long as they match the expected form-column stretching behavior.
Applied to files:
src/ui/components/SettingsView/features/NavigationLayout.spec.tsxsrc/ui/components/SettingsView/features/MenuBar.tsxsrc/ui/components/SettingsView/features/NavigationLayout.tsx
📚 Learning: 2026-06-26T18:14:13.838Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/ui/components/SettingsView/features/ToggleField.tsx:1-8
Timestamp: 2026-06-26T18:14:13.838Z
Learning: In Rocket.Chat Electron App settings field UIs that use the Fuselage three-tier pattern, keep the `FieldLabel` / `FieldDescription` / `FieldHint` structure separate. Use `FieldDescription` for the regular secondary body text, and reserve `FieldHint` for the smaller, dimmer subline content (e.g., restart caveats). Do not collapse `FieldDescription` and `FieldHint` into a single hint tier, as this violates the intended UXDQA spec.
Applied to files:
src/ui/components/SettingsView/features/NavigationLayout.spec.tsxsrc/ui/components/SettingsView/features/MenuBar.tsxsrc/ui/components/SettingsView/features/NavigationLayout.tsx
📚 Learning: 2026-05-19T20:49:24.859Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat.Electron PR: 3329
File: src/ui/reducers/e2ePdfPreviewSizeLimit.ts:14-16
Timestamp: 2026-05-19T20:49:24.859Z
Learning: In Rocket.Chat.Electron’s reducer files under src/ui/reducers/, reducers should not re-implement validation for action payloads. Assume the caller (UI component or dispatch site) has already validated the action payload and type/shape; reducers should trust the payload and update state directly. If validation is needed, add it at the dispatch site/caller rather than inside the reducer.
Applied to files:
src/ui/reducers/navigationLayout.spec.tssrc/ui/reducers/navigationLayout.ts
📚 Learning: 2026-05-19T20:49:24.859Z
Learnt from: nazabucciarelli
Repo: RocketChat/Rocket.Chat.Electron PR: 3329
File: src/ui/reducers/e2ePdfPreviewSizeLimit.ts:14-16
Timestamp: 2026-05-19T20:49:24.859Z
Learning: In the Rocket.Chat.Electron UI reducers under src/ui/reducers/, do not add/repeat input validation for action payloads inside reducers. Follow the existing codebase pattern: validate the action payload in the caller (e.g., the UI component or dispatch site) before dispatching. Reducers should trust the incoming payload and apply it directly to state. If adding/updating a reducer, ensure the corresponding caller performs the necessary validation (e.g., check numeric constraints like !isNaN(value) && value > 0 before dispatching the action).
Applied to files:
src/ui/reducers/navigationLayout.spec.tssrc/ui/reducers/navigationLayout.ts
📚 Learning: 2026-06-26T18:14:15.295Z
Learnt from: jeanfbrito
Repo: RocketChat/Rocket.Chat.Electron PR: 3358
File: src/i18n/it-IT.i18n.json:39-42
Timestamp: 2026-06-26T18:14:15.295Z
Learning: In the i18n JSON files, the translation key `minimizeOnClose.disabledHint` is intentionally displayed when `isTrayIconEnabled` is true and the minimize-on-close toggle is disabled. The hint text should therefore instruct the user to disable the tray icon to make the setting available. During reviews, don’t “correct” this translation for seeming mismatches with the toggle state—first confirm it matches the component’s intended behavior; only update the wording if the underlying product logic/UX requirement changes.
Applied to files:
src/i18n/en.i18n.json
🪛 ast-grep (0.44.1)
src/ui/preload/sidebar.ts
[warning] 20-25: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: style.innerHTML = .sidebar { padding-top: ${isTrafficLightsCovered ? 0 : '10px'} !important; transition: padding-top 230ms ease-in-out !important; }
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(dom-content-modification)
[warning] 20-25: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: style.innerHTML = .sidebar { padding-top: ${isTrafficLightsCovered ? 0 : '10px'} !important; transition: padding-top 230ms ease-in-out !important; }
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation
(unsafe-html-content-assignment)
🔇 Additional comments (55)
src/ui/components/SettingsView/features/NavigationLayout.tsx (2)
51-54: Disable logic matches native menu radio state.
isWorkspaceTabsDisabledmirrors theenabledcondition used for theworkspaceTabsradio inmenuBar.ts(process.platform !== 'linux' || isMenuBarEnabled || navigationLayout === 'tabs'), keeping the settings UI and native menu consistent.
1-94: LGTM!src/ui/components/SettingsView/features/NavigationLayout.spec.tsx (1)
1-141: LGTM!Test coverage aligns well with the component's disable/dispatch logic, including the Linux-with-hidden-menu-bar edge case.
src/ui/components/SettingsView/features/MenuBar.tsx (1)
20-22: Toggle gate correctly ported tonavigationLayout.
canToggle/descriptionnow key offnavigationLayout === 'sidebar'instead of the removedisSideBarEnabled, consistent withmenuBar.ts'sshowMenuBarenablement (!isMenuBarEnabled || navigationLayout === 'sidebar').Also applies to: 37-37
src/ui/components/SettingsView/GeneralTab.tsx (1)
10-12: LGTM!Swap of
SideBarforNavigationLayoutand gatingMenuBarto!isDarwin && !isWin32is consistent with the PR's Windows meatball-menu approach for exposing the layout toggle.Also applies to: 25-33
src/ui/components/SettingsView/SettingsView.tsx (1)
1-94: LGTM!Back-button removal is clean and consistent with the AI summary; no dangling references to
IconButton,dispatch, orDOWNLOADS_BACK_BUTTON_CLICKEDremain in the shown code.src/i18n/en.i18n.json (1)
308-314: LGTM!New keys (
settings.options.navigation.*,menus.checkForUpdates,menus.workspaceTabs/workspaceBar/nextWorkspace/previousWorkspace,sidebar.item.addWorkspace) match the strings referenced inNavigationLayout.tsx,MenuBar.tsx, and the menu bar snippets provided.Also applies to: 425-425, 452-455, 512-513
src/app/PersistableValues.ts (1)
121-127: LGTM!Also applies to: 240-244
src/app/__tests__/PersistableValues.spec.ts (1)
19-37: LGTM!src/ui/common.ts (1)
5-6: LGTM!src/ui/actions.ts (1)
4-4: LGTM!Also applies to: 36-37, 138-142, 175-180, 201-201, 298-300, 351-354
src/store/rootReducer.ts (1)
53-53: LGTM!Also applies to: 92-92
src/ui/reducers/navigationLayout.ts (1)
16-33: LGTM!src/ui/reducers/navigationLayout.spec.ts (1)
10-66: LGTM!Also applies to: 87-111
src/app/selectors.ts (1)
1-105: 🗄️ Data Integrity & IntegrationAll persistable keys are covered by the split selectors.
src/ui/components/SideBar/index.tsx (2)
21-30: LGTM!
41-41: LGTM!src/ui/components/SideBar/index.spec.tsx (2)
46-48: LGTM!
116-118: LGTM!src/ui/components/ServersView/ServerPane.tsx (1)
13-13: LGTM!Also applies to: 30-30, 43-43
src/ui/components/ServersView/index.tsx (1)
1-12: LGTM!Also applies to: 29-29
src/ui/components/SideBar/ServerButton.tsx (1)
28-28: LGTM!Also applies to: 105-105
src/ui/components/utils/getServerInitials.ts (1)
1-11: LGTM!src/ui/preload/sidebar.ts (2)
4-7: LGTM!
20-23: LGTM!src/app/main/data.ts (1)
167-176: LGTM!src/app/main/data.spec.ts (1)
20-20: LGTM!Also applies to: 53-135, 176-181, 199-204
src/ui/main/menuBar.ts (2)
389-429: 🎯 Functional CorrectnessConfirm the missing
enabledguard onworkspaceBaris intentional.
workspaceTabsis disabled on Linux when the menu bar is hidden and layout isn't already tabs (preventing a mid-session switch that would strand the user without menu access, since the tabs layout has no equivalent in-strip meatball button on Linux).workspaceBarhas no equivalentenabledguard at all, so it's always clickable regardless of platform/menu-bar state. If this asymmetry is deliberate (switching to sidebar never removes menu access), it'd help to note that; otherwise this may be a missed guard.
95-167: LGTM!Also applies to: 209-244, 365-371, 545-584, 870-950, 962-982
src/ui/main/serverView/index.ts (1)
31-31: LGTM!Also applies to: 559-561, 620-630
src/ui/components/TabBar/useTabBarLayout.ts (1)
9-42: LGTM!src/ui/components/TabBar/useTabBarLayout.spec.ts (1)
1-81: LGTM!src/ui/components/TabBar/MeatballMenuButton.spec.tsx (1)
1-93: LGTM!src/ui/components/TabBar/index.spec.tsx (1)
1-259: LGTM!src/ui/main/rootWindow.ts (3)
28-34: LGTM!Also applies to: 82-84
577-589: LGTM!
327-363: 🚀 Performance & ScalabilityConfirm
WINDOW_CONTROLS_*actions are only dispatched from platform-appropriate UI. The handlers are registered unconditionally here; ifWindowControlscan mount on macOS/Linux, the native window controls would duplicate these mutations.src/ui/components/TabBar/CloseGlyph.tsx (1)
1-7: 📐 Maintainability & Code Quality | 💤 Low valueVerify Fuselage doesn't already provide a close icon before adding a custom glyph.
This introduces a custom SVG icon. Custom window-chrome glyphs mimicking native OS controls may be a legitimate exception, but per coding guidelines Fuselage components/icons should be preferred unless they don't cover this need.
As per coding guidelines: "MANDATORY: Use Fuselage components for all UI work. Only create custom components when Fuselage doesn't provide what's needed."
Source: Coding guidelines
src/ui/components/TabBar/MaximizeGlyph.tsx (1)
1-15: LGTM!src/ui/components/TabBar/MinimizeGlyph.tsx (1)
1-8: LGTM!src/ui/components/TabBar/RestoreGlyph.tsx (1)
12-15: 📐 Maintainability & Code Quality | 💤 Low valueVerify
--rcx-color-surface-tintis a valid design token.This CSS variable is used directly with a hardcoded fallback rather than a value confirmed against Fuselage's theme tokens.
As per coding guidelines: "Import UI components from
@rocket.chat/fuselageand checkTheme.d.tsfor valid color tokens."Source: Coding guidelines
src/ui/components/TabBar/WindowControls.tsx (1)
1-73: LGTM!src/ui/components/TabBar/WindowControls.spec.tsx (1)
1-137: LGTM!src/ui/components/TabBar/MeatballMenuButton.tsx (2)
1-8: LGTM!Also applies to: 26-51
26-51: 🎯 Functional CorrectnessSolo-Alt handling needs a forwarding path from embedded workspace views. If the active workspace view consumes
Alt, the shellwindowlisteners won’t see it, so this should either be forwarded from the workspaceWebContentsor called out as an intentional deferred gap.src/ui/components/TabBar/WorkspaceTab.tsx (2)
1-65: LGTM!Also applies to: 87-145
66-69: 🎯 Functional Correctness | ⚡ Quick winShortcut label format differs from the existing sidebar tooltip.
WorkspaceTabrenders⌘1/Ctrl+1, whileServerButton's equivalent tooltip uses⌘+1/^+1. Users switching between Workspace Bar and Workspace Tabs will see two different notations for the same accelerator.💡 Align formatting with the existing sidebar convention
- const shortcutSuffix = - shortcutNumber && Number(shortcutNumber) >= 1 && Number(shortcutNumber) <= 9 - ? ` (${isDarwin ? '⌘' : 'Ctrl+'}${shortcutNumber})` - : ''; + const shortcutSuffix = + shortcutNumber && Number(shortcutNumber) >= 1 && Number(shortcutNumber) <= 9 + ? ` (${isDarwin ? '⌘' : '^'}+${shortcutNumber})` + : '';src/ui/components/TabBar/WindowsTitleBar.tsx (1)
1-25: LGTM!src/ui/components/TabBar/index.tsx (2)
1-67: LGTM!Also applies to: 105-140, 152-157
68-103: 🎯 Functional Correctness | ⚡ Quick winAdd-workspace button nested inside
role="tablist"breaks tab-list ARIA semantics.The add button lives inside the same
role='tablist'container as the tabs but isn't part of the[role="tab"]roving-tabindex set built inhandleTabListKeyDown(Lines 69-71). It keeps its own nativetabIndex, so it's reachable via normal Tab-key navigation from inside a tablist — a pattern most screen readers/AT and axe-core'saria-required-children/tablist rules flag, since a tablist's interactive descendants should all be tabs. Consider moving the add button outside<TabList>(as a sibling within<Strip>), or removing it from the tab flow by rendering it after the tablist closes.♿ Move the add button outside the tablist
<TabList ref={tabListRef} role='tablist' aria-label={t('tabBar.workspaces')} onKeyDown={handleTabListKeyDown} > {visibleServers.map((server, index) => { ... })} - {isAddNewServersEnabled && ( - <AddButtonWrapper> - <IconButton - small - icon='plus' - title={t('tabBar.addWorkspace')} - onClick={handleAddServerButtonClicked} - /> - </AddButtonWrapper> - )} </TabList> + {isAddNewServersEnabled && ( + <AddButtonWrapper> + <IconButton + small + icon='plus' + title={t('tabBar.addWorkspace')} + onClick={handleAddServerButtonClicked} + /> + </AddButtonWrapper> + )} <DragSpacer />Also applies to: 109-151
src/ui/components/TabBar/styles.tsx (3)
1-16: LGTM!Also applies to: 21-81, 129-183
78-78: 🎯 Functional CorrectnessCheck Electron/Chromium support for container queries.
container-type: inline-sizeand@containerrequire Chromium 105+, so this depends on the app’s minimum Electron version.
8-21: 🎯 Functional Correctness
--rcx-color-*variables are valid Fuselage tokens; the fallback warning is incorrect.> Likely an incorrect or invalid review comment.src/ui/components/Shell/index.tsx (1)
20-23: LGTM!Also applies to: 42-44, 81-105
src/ui/components/Shell/index.spec.tsx (1)
55-85: LGTM!Also applies to: 163-184, 269-339
| <Wrapper | ||
| isVisible={isSelected} | ||
| {...(isTabPanel && { | ||
| 'id': getServerPanelId(serverUrl), | ||
| 'role': 'tabpanel', | ||
| 'aria-labelledby': getServerTabId(serverUrl), | ||
| 'hidden': !isSelected, | ||
| })} | ||
| > |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how useTabBarLayout/TabBar handles overflowed servers to confirm every server gets a rendered tab element with a stable id.
fd -t f 'useTabBarLayout|WorkspaceTab|index.tsx' src/ui/components/TabBar
cat -n src/ui/components/TabBar/useTabBarLayout.ts 2>/dev/null
cat -n src/ui/components/TabBar/index.tsx 2>/dev/nullRepository: RocketChat/Rocket.Chat.Electron
Length of output: 9518
Keep a tab element mounted for overflowed servers
TabBar renders only visibleServers, so condensed workspaces lose their role="tab" element while ServerPane still points aria-labelledby at getServerTabId(serverUrl). Keep an id-bearing tab in the DOM for every server, or update the panel labeling strategy for overflowed tabs.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/components/ServersView/ServerPane.tsx` around lines 210 - 218, The tab
panel in ServerPane still references getServerTabId(serverUrl), but overflowed
servers no longer have a mounted role="tab" element because TabBar only renders
visibleServers. Update the tab labeling approach so every server keeps an
id-bearing tab in the DOM, or adjust ServerPane’s aria-labelledby/tabpanel
wiring to avoid pointing at missing tab elements when a server is condensed.
| it('should use default state when navigationLayout not in payload', () => { | ||
| const action: ActionOf<typeof APP_SETTINGS_LOADED> = { | ||
| type: APP_SETTINGS_LOADED, | ||
| payload: {}, | ||
| }; | ||
|
|
||
| expect(navigationLayout('tabs', action)).toBe('tabs'); | ||
| }); | ||
|
|
||
| it('should handle undefined payload gracefully', () => { | ||
| const action: ActionOf<typeof APP_SETTINGS_LOADED> = { | ||
| type: APP_SETTINGS_LOADED, | ||
| payload: {} as any, | ||
| }; | ||
|
|
||
| expect(navigationLayout('tabs', action)).toBe('tabs'); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Duplicate/misleadingly named test — doesn't actually test an undefined payload.
The test titled "should handle undefined payload gracefully" (lines 77-84) passes payload: {} — identical to the preceding test — not payload: undefined. It doesn't exercise the case its name describes, and the reducer's destructuring (const { navigationLayout = state } = action.payload;) would actually throw on a truly undefined payload.
✅ Suggested fix
- it('should handle undefined payload gracefully', () => {
- const action: ActionOf<typeof APP_SETTINGS_LOADED> = {
- type: APP_SETTINGS_LOADED,
- payload: {} as any,
- };
-
- expect(navigationLayout('tabs', action)).toBe('tabs');
- });
+ it('should handle undefined payload gracefully', () => {
+ const action = {
+ type: APP_SETTINGS_LOADED,
+ payload: undefined,
+ } as unknown as ActionOf<typeof APP_SETTINGS_LOADED>;
+
+ expect(() => navigationLayout('tabs', action)).not.toThrow();
+ });Note: if action.payload can genuinely be undefined at runtime, the reducer itself needs a guard (e.g. action.payload ?? {}) for this test to pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/ui/reducers/navigationLayout.spec.ts` around lines 68 - 84, The test case
named “should handle undefined payload gracefully” in navigationLayout.spec is
misleading because it still passes an empty object, so it does not cover the
undefined-payload path. Update that test to actually send an undefined payload
for APP_SETTINGS_LOADED, and verify the navigationLayout reducer still returns
the default state; if the reducer’s destructuring in navigationLayout cannot
handle undefined, add a guard in the reducer first so it safely falls back
before destructuring.
- data.spec.ts: two legacy-migration tests didn't pin navigationLayout
and ran under the ambient default from mockInitialValues ('tabs'),
colliding with the new Linux tabs-mode menu bar recovery guard; pin
navigationLayout: 'sidebar' to match their pre-tabs migration intent.
Also switch all mockSelect setups from mockReturnValueOnce to
mockReturnValue so each test's mock is not order-dependent on a
single-call queue, which was intermittently leaking the default
mockInitialValues into unrelated tests in CI (macOS + Windows)
- Shell/index.spec.tsx: the sidebar-layout test asserted on
darwin-only TopBar without pinning process.platform, so it only
passed by accident on macOS runners and failed on Linux/Windows CI;
pin platform to darwin like the sibling win32-chrome tests do
Temporary — will be reverted once root cause of the navigationLayout leak on CI (but not reproducible locally after fresh clone, full suite, coverage on/off, Node 22/24, CI env vars) is identified.
…e failure" This reverts commit 19c7b2a.
…a on CI
Root cause found via CI diagnostic logging (now reverted): mockSelect
correctly returned each test's override, but the dispatched payload
matched shapes only the REAL, unmocked electron-store persistence
layer could produce. jest.doMock('./persistence'/'fs'/'electron', ...)
inside beforeEach are no-ops here — data.ts already statically
imported those modules at file-load time, before any doMock call, so
the bindings never pointed at the mocks. getPersistedValues() spreads
after the mocked select() result in mergePersistableValues
(...initialValues, ...electronStoreValues), so on any machine with a
real persisted electron-store config.json (apparently present in
GitHub's CI runners, absent in fresh local clones), real disk state
silently overrode the test's intended mock — explaining why this was
100% reproducible on every CI runner/OS and 0% reproducible locally
across fresh clones, Node 22/24, and coverage on/off.
Fix: hoist the mocks to real jest.mock() calls (matching the working
jest.mock('../../store') pattern) so getPersistedValues, fs, and
electron are deterministically mocked regardless of what's on disk.
Also mock '../../logging' since the real electron mock surfaced a
'app.on is not a function' fallback error from the logger's real
init code, previously masked by the same dead-mock bug.
Linux installer download |
macOS installer download |
- Fix i18n plural key: unreadMessage_plural -> unreadMessage_one/_other (i18next 23 defaults to v4/CLDR pluralization, no compatibilityJSON v3 flag set anywhere; the old _plural suffix silently never matched, falling back to the singular string for any count) - Fix NavigationLayout settings group title label incorrectly bound via htmlFor to the 'Workspace Tabs' radio specifically, making the section heading silently select tabs when clicked despite each radio already having its own correctly-scoped label - Fix getServerDomId id collisions: sanitize() stripped punctuation aggressively enough that distinct URLs (e.g. with/without trailing slash) could produce identical workspace-tab-*/workspace-panel-* ids, breaking the aria-controls/aria-labelledby pairing; append a stable hash of the full URL - Remove navigationLayout.spec.ts test that claimed to cover an undefined APP_SETTINGS_LOADED payload but passed an empty object identical to the preceding test; the action's payload type is never actually undefined by contract, so the described case can't occur Rejected: ServerPane.tsx aria-labelledby referencing a condensed-out tab — verified against useTabBarLayout's computeVisibleServers, which always force-includes the active/selected server in visibleServers, so the only panel with a real ARIA reference (the visible, non-hidden one) always has a matching mounted tab element.
1 participant
CORE-2312
Summary
Introduces a horizontal, browser-style tab bar at the top of the window as a new workspace switcher, alongside the existing vertical workspace bar. Workspace Tabs is the new default on first launch and after upgrade; the two displays are mutually exclusive and toggled from the View menu (macOS native menu bar) or the meatball dropdown (Windows). User selection persists across restarts.
Key changes
TabBarcomponent tree:WorkspaceTab,WindowControls,MeatballMenuButton,useTabBarLayout— Chrome-style progressive tab condensing (52px minimum width, name truncates then drops to icon-only) with a reserved 44px drag region that never shrinksnavigationLayoutreducer + persistence tracks the bar-vs-tabs choice; migrates cleanly from existing sidebar server orderMenu.popup()meatball dropdown since the native window menu bar is removed thereuseKeyboardShortcuts/useSortingmoved out ofSideBarinto sharedcomponents/hooksso both displays reuse the same drag-reorder and shortcut logicaria-controls/aria-labelledby), arrow-key navigation between tabs, visible focus ringWindows window controls — implementation note (AC14)
Window controls (minimize / maximize-restore / close) on Windows are custom-drawn inside the tab strip (
WindowControls.tsx), not Electron's nativetitleBarOverlay. This was the faster path to match the visual target, but it means the following native behaviors are not currently implemented and would need follow-up work if wanted:titleBarOverlayMinimize/maximize/restore/close all work via direct
BrowserWindowcalls and remain functional in fullscreen and maximized states; only the above native affordances are the tradeoff of the custom-drawn approach.Out of scope
Test plan
npx tsc --noEmitcleanyarn lintcleanSummary by CodeRabbit